Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

first round of updates to disable type assertions #2879

Merged

Conversation

rsun19
Copy link
Contributor

@rsun19 rsun19 commented Jun 5, 2024

Closes: RHOAIENG-4336

Description

We would like to limit the use of type assertions as much as possible. However, a large amount of these assertions are made in a type-cast like way making it very tricky to type check them. We may need to resort to using as in the type guard in those cases.

How Has This Been Tested?

npm run test

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot requested review from Gkrumbach07 and mturley June 5, 2024 15:21
@rsun19 rsun19 changed the title first round of updates to disable type assertions [WIP] first round of updates to disable type assertions Jun 5, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jun 5, 2024
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch 2 times, most recently from 4ac45ef to 8011609 Compare June 6, 2024 20:51
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch from 8011609 to 1b5430c Compare June 7, 2024 19:47
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 8, 2024
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch 2 times, most recently from 2b1b073 to 1112c0d Compare June 10, 2024 15:43
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 10, 2024
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch 3 times, most recently from d4b787e to cc0d111 Compare June 10, 2024 17:34
@rsun19 rsun19 changed the title [WIP] first round of updates to disable type assertions first round of updates to disable type assertions Jun 10, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jun 10, 2024
frontend/src/components/DocCardBadges.tsx Outdated Show resolved Hide resolved
frontend/src/components/DocCardBadges.tsx Outdated Show resolved Hide resolved
frontend/src/components/OdhDocListItem.tsx Outdated Show resolved Hide resolved
frontend/src/concepts/areas/utils.ts Outdated Show resolved Hide resolved
frontend/src/concepts/dashboard/DashboardSearchField.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/NavData.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/useDraggableTable.ts Outdated Show resolved Hide resolved
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch 3 times, most recently from e977cc1 to af884c1 Compare June 13, 2024 17:51
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 13, 2024
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch 2 times, most recently from 933956e to 0722fd0 Compare June 13, 2024 18:21
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 13, 2024
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch from c9cd7ed to e3f487a Compare June 13, 2024 18:31
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 71.50538% with 53 lines in your changes missing coverage. Please review.

Project coverage is 78.45%. Comparing base (3d62686) to head (666ccb4).

Current head 666ccb4 differs from pull request most recent head 0af9464

Please upload reports for the commit 0af9464 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2879      +/-   ##
==========================================
- Coverage   78.54%   78.45%   -0.10%     
==========================================
  Files        1138     1139       +1     
  Lines       24146    24224      +78     
  Branches     6098     6151      +53     
==========================================
+ Hits        18966    19005      +39     
- Misses       5180     5219      +39     
Files Coverage Δ
frontend/src/api/errorUtils.ts 100.00% <100.00%> (ø)
frontend/src/api/modelRegistry/errorUtils.ts 100.00% <100.00%> (ø)
frontend/src/api/pipelines/errorUtils.ts 100.00% <100.00%> (ø)
frontend/src/api/prometheus/serving.ts 95.91% <100.00%> (+0.79%) ⬆️
frontend/src/api/trustyai/errorUtils.ts 85.71% <100.00%> (-1.79%) ⬇️
frontend/src/app/AppContext.ts 100.00% <100.00%> (ø)
frontend/src/components/DocCardBadges.tsx 95.23% <100.00%> (ø)
...end/src/components/FilterSidePanelCategoryItem.tsx 100.00% <100.00%> (ø)
frontend/src/components/OdhDocListItem.tsx 75.60% <100.00%> (ø)
frontend/src/components/ToastNotification.tsx 65.00% <100.00%> (+1.84%) ⬆️
... and 77 more

... and 21 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d62686...0af9464. Read the comment docs.

@rsun19 rsun19 force-pushed the eslint-disable-assertion branch from e3f487a to 4c353b9 Compare June 13, 2024 20:09
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch 2 times, most recently from 10f43ce to 3103b0e Compare June 14, 2024 17:35
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 14, 2024
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch from 3103b0e to 70fae10 Compare June 14, 2024 17:39
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 14, 2024
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch from 70fae10 to 4a3e920 Compare June 14, 2024 18:12
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch from 4a3e920 to 538ba4d Compare June 14, 2024 19:40
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch 2 times, most recently from 5d5b8c7 to cb9ac68 Compare June 18, 2024 17:51
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch 3 times, most recently from 666ccb4 to 9658cae Compare June 21, 2024 14:15
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch from 9658cae to bff3169 Compare June 21, 2024 14:30
@rsun19 rsun19 force-pushed the eslint-disable-assertion branch from bff3169 to 0af9464 Compare June 21, 2024 14:36
@christianvogt
Copy link
Contributor

/lgtm
/approve

Copy link
Contributor

openshift-ci bot commented Jun 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 8808e8d into opendatahub-io:main Jun 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants